Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse qualified paths in type params and provide custom diagnostic #60323

Closed
wants to merge 2 commits into from

Conversation

estebank
Copy link
Contributor

Fix #26271.

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 27, 2019
@Centril
Copy link
Contributor

Centril commented Apr 27, 2019

When #52662 (cc) becomes stable we can suggest that instead.

@michaelwoerister
Copy link
Member

@petrochenkov Do you have opinions on this? Do you even want to review it maybe? :)

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 29, 2019

I'm generally unhappy about parsing random specific constructions in random positions just because someone made a ticket.

Perhaps the "consider adding an explicit lifetime bound ::TrSubtype: 'a" suggestion should instead mention where and point to a better code location, so mistakes like that don't have opportunity to occur.

@estebank
Copy link
Contributor Author

@petrochenkov beyond the specifics of this ticket in particular, I believe the parser should be able to parse a superset of valid code for things that are either correct in other languages (a or b -> a || b) or that seem like valid syntax given how other parts of the syntax work (let x = [0]; x.0/let x: &'a usize = &'a 3;). Writing a qpath in a type argument list seems like something that fits this later behavior, something someone familiar with Rust syntax but not all its intricacies would try. Also I would point out that if there's one ticket it probably means there is a larger multiple of people attempting this and being confused. It is true that the chain of thought that ended in this code is no longer as problematic, but would still want the compiler to catch it.

@Centril
Copy link
Contributor

Centril commented Apr 29, 2019

Also I would point out that if there's one ticket it probably means there is a larger multiple of people attempting this and being confused.

I don't have an opinion on the specifics of this PR; but in general, is this really true?

@estebank
Copy link
Contributor Author

@Centril I have a few dozen coworkers writing Rust. Anecdotally, I have only seen three people in a little less than a year file a rustc bug for something they spent a day or more trying to figure out, and two of them only did so because I encouraged them to do so. The amount of times they have approached others to ask what the hell the compiler is saying is somewhere around 50x.

I can certainly see that there are some really obscure corner cases (like this one) that might not be worth complicating the parser for, but any ticket that has either "+1" style comments or duplicates tells me that the amount of people actually encountering that problem is probably >100, and those numbers are only going to increase as the community expands. Not every diagnostic is born equal, but just as much as following the Pareto curve let's us focus most of our attention to the cases that come up 80% the time, I think we should give some attention to the long tail cases, particularly if .

I'm also empathetic to needing to clean up the happy path code and make diagnostics code unobtrusive. This is something that myself and others are planning to work on once we have a large enough chunk of time to do so: it's at the top of our priorities for this year.

@Centril
Copy link
Contributor

Centril commented Apr 29, 2019

I can certainly see that there are some really obscure corner cases (like this one) that might not be worth complicating the parser for, but any ticket that has either "+1" style comments or duplicates tells me that the amount of people actually encountering that problem is probably >100, and those numbers are only going to increase as the community expands.

@estebank If there are duplicates and "me too" comments on such diagnostics issues then I think there's a strong case for the improvement benefiting many people. I mainly think that it's not necessarily true that if there's a single issue reporting something obscure, that it will affect many folks and that it therefore many not deserve special attention.

I'm also empathetic to needing to clean up the happy path code and make diagnostics code unobtrusive. This is something that myself and others are planning to work on once we have a large enough chunk of time to do so: it's at the top of our priorities for this year.

❤️

@petrochenkov
Copy link
Contributor

@estebank

I believe the parser should be able to parse a superset of valid code

Agree entirely.

beyond the specifics of this ticket in particular

My concerns are mostly about particularities and specific PRs, not about improving error recovery in general.

Writing a qpath in a type argument list seems like something that fits this later behavior

Here's where I disagree.
Why does qpaths get special treatment, but e.g. struct Bar<'a, T, "hello"> doesn't?
Because another diagnostics recommended it, but didn't recommend "hello".
From my point of view it would be better to fix the root issue (that another diagnostics positioning), than consequences.

Also I would point out that if there's one ticket it probably means there is a larger multiple of people attempting this and being confused.

That's arguable.
Some people report any perceived minor issue, some don't report anything unless it ruins their business (and, of course, there's a whole spectre in between).
So, an individual issue without confirmations from other people may turn out a whim of an enthusiastic reporter.
In case of #26271 there is one confirmation, so I'm ready to believe that the issue is real, but would still prefer to address it in other way.

Even if an issue is real and closing it is a positive, we should still try balance it with less noticeable and more long term goals, and attempt to 1) fix it as non-intrusively as possible (only wording, ideally) and 2) generalize to increase the positive effect (while not increasing intrusiveness).

In case of this PR a wording-only solution (mentioning where clauses) + perhaps adjusting position of the suggestion would be the more balanced solution, IMO.

(I'm not going to go on a crusade and block PRs that provide unbalanced solutions to diagnostic issues, but I'll probably still going to nag people about this from time to time, because the priorities need to be adjusted more in favor of the compiler maintainability.)

@Centril
Copy link
Contributor

Centril commented May 1, 2019

@estebank Perhaps leave a FIXME for the replacement when the RFC stabilizes?

@michaelwoerister
Copy link
Member

r? @petrochenkov, if you don't mind.

@petrochenkov
Copy link
Contributor

r? @petrochenkov, if you don't mind.

Oh no.

@estebank
Copy link
Contributor Author

estebank commented May 2, 2019

@petrochenkov I have PR I'm working on to split impl Parser in two/three traits, at least one for happy path and one exclusively for diagnostics. Would you prefer if we revisited this (and the other PR) after I have done that?

@petrochenkov
Copy link
Contributor

Yeah, that would be nice, this is not an urgent issue.
(Closing.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hint for a lifetime binding of a type in a trait suggests invalid syntax [E0309]
5 participants